Conversation
|
I have a very different implementation on my working tree which rather than to build a mutable Bazaar Reply, declares a Product Registry whereof api snapshot is one of the many sources ingested onto the hash map. I'll commit it to my fork in a sec so you can check which path you'd prefer |
https://github.com/0xar-ds/Bazaar-Utils/tree/refactor/bazaar-data-attempt Every source is factored in currently, most consumers currently remain as a
|
|
lmk if you want me to get that tree/branch onto a more proper commit history on some branch of this repo, if you'd rather iterate/choose that |
|
@0xar-ds right now I am still looking it over... it's a giant update 😆 . Definitely we will use some of it if not all, but I would prefer if we could split it into smaller pr's where possible |
5e84ca6 to
77b5e81
Compare
…ductOrder with ref to Order
|
Rebasing |
f464821 to
e62e2a1
Compare
There was a problem hiding this comment.
Pull request overview
Refactors Bazaar market-data handling by splitting responsibilities out of BazaarDataManager and converting Hypixel API objects into internal wrapper types, while also reorganizing some storage/util classes and updating call sites to use the new APIs.
Changes:
- Introduces wrapper types (
CustomBazaarReply,ProductData,ProductOrder) plusAPIConversionUtilto convertSkyBlockBazaarReplyinto an internal snapshot model. - Extracts pricing / product-id lookup helpers into
BazaarDataUtiland moves conversions caching intoResourceManager. - Renames/moves API + storage classes and updates call sites (
APIUtils→APIUtil,*.data.*Storage→utils.storage.*Storage,BazaarDataManager.*→BazaarDataUtil.*).
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/github/mkram17/bazaarutils/utils/storage/UserOrdersStorage.java | Moves storage class into utils.storage package. |
| src/main/java/com/github/mkram17/bazaarutils/utils/storage/BookmarksStorage.java | Moves storage class into utils.storage package. |
| src/main/java/com/github/mkram17/bazaarutils/utils/storage/BazaarLimitsStorage.java | Moves storage class into utils.storage package. |
| src/main/java/com/github/mkram17/bazaarutils/utils/minecraft/PlayerSlots.java | Switches product-id lookup to BazaarDataUtil. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/market/order/OrderUtil.java | Updates pricing lookups to use BazaarDataUtil. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/market/order/OrderUpdater.java | Updates UserOrdersStorage import to new package. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/market/order/OrderInfo.java | Uses BazaarDataUtil for product-id validation/lookup and order-count lookup. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/market/order/Order.java | Updates UserOrdersStorage import to new package. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/gui/BazaarScreenHandler.java | Switches product-id lookup to BazaarDataUtil. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/data/wrappers/ProductOrder.java | New wrapper for a summarized price level (price/volume/orders). |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/data/wrappers/ProductData.java | New wrapper grouping buy/sell books for a product. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/data/wrappers/CustomBazaarReply.java | New wrapper reply type replacing direct SkyBlockBazaarReply usage in the app. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/data/wrappers/APIConversionUtil.java | New conversion pipeline from Hypixel reply → internal wrapper model. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/data/BazaarDataUtil.java | New utility surface for top-price, order-count, and product-id resolution/validation. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/data/BazaarDataSettings.java | New settings holder for polling/backoff constants. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/data/BazaarDataManager.java | Updates polling loop to store CustomBazaarReply and delegates conversion/utilities. |
| src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/SignInputHelper.java | Updates storage + price/id lookups to new packages/APIs. |
| src/main/java/com/github/mkram17/bazaarutils/utils/ResourceManager.java | Hosts conversions cache + loader and exposes it for BazaarDataUtil. |
| src/main/java/com/github/mkram17/bazaarutils/utils/APIUtil.java | Renames/moves API helper (APIUtils→APIUtil) under utils. |
| src/main/java/com/github/mkram17/bazaarutils/features/notification/OutbidOrderHandler.java | Updates UserOrdersStorage import to new package. |
| src/main/java/com/github/mkram17/bazaarutils/features/gui/overlays/PriceCharts.java | Switches product-id lookup to BazaarDataUtil. |
| src/main/java/com/github/mkram17/bazaarutils/features/gui/overlays/BazaarLimitsVisualizer.java | Updates BazaarLimitsStorage import to new package. |
| src/main/java/com/github/mkram17/bazaarutils/features/gui/buttons/bookmarks/BookmarkUtil.java | Updates BookmarksStorage import to new package. |
| src/main/java/com/github/mkram17/bazaarutils/events/handler/ChatHandler.java | Updates UserOrdersStorage import to new package. |
| src/main/java/com/github/mkram17/bazaarutils/events/handler/BazaarChatEventHandler.java | Updates UserOrdersStorage import to new package. |
| src/main/java/com/github/mkram17/bazaarutils/events/BazaarDataUpdateEvent.java | Event now carries CustomBazaarReply instead of SkyBlockBazaarReply. |
| src/main/java/com/github/mkram17/bazaarutils/commands/DeveloperCommands.java | Switches product-id lookup to BazaarDataUtil. |
| private static void handleFetchFailure(String messagePrefix, boolean includeFailureCount) { | ||
| int failureCount = includeFailureCount ? consecutiveFailures.incrementAndGet() : consecutiveFailures.get(); | ||
| String message = includeFailureCount ? messagePrefix + " (failures=" + failureCount + ")" : messagePrefix; | ||
| Util.notifyError(message, new Throwable()); | ||
| scheduleFailureRetry(); |
There was a problem hiding this comment.
handleFetchFailure discards the real underlying exception by always calling Util.notifyError(message, new Throwable()). This makes debugging API failures much harder and loses the original stack trace. Pass the actual throwable (or a meaningful exception for non-throwable failure cases) through to notifyError.
src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/data/BazaarDataUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/data/wrappers/APIConversionUtil.java
Show resolved
Hide resolved
| // Double-checked guard avoids repeated JSON parsing on the hot path. | ||
| synchronized (BazaarDataManager.class) { | ||
| if (conversionsLoaded) { | ||
| return; |
There was a problem hiding this comment.
ensureConversionsLoaded() synchronizes on BazaarDataManager.class, which couples ResourceManager’s cache locking to an unrelated class and risks unexpected contention/deadlocks if BazaarDataManager ever also synchronizes while calling into ResourceManager. Use ResourceManager.class or a dedicated private lock object for this cache instead.
There was a problem hiding this comment.
@copilot edit this pull request to fix the issues you noted in comments here.
src/main/java/com/github/mkram17/bazaarutils/utils/bazaar/data/wrappers/ProductData.java
Outdated
Show resolved
Hide resolved
| if (throwable != null || !reply.isSuccess()) { | ||
| handleFetchFailure( | ||
| "Fetch failure (" + throwable.getClass().getSimpleName() + "). Retry in " + BAZAAR_DATA_SETTINGS.FAILURE_RETRY_MS + "ms", |
There was a problem hiding this comment.
fetchOnce() can throw a NullPointerException when the API completes with reply == null (since reply.isSuccess() is called) or when the reply is unsuccessful but throwable == null (since throwable.getClass() is used for the message). Add explicit null checks for reply/throwable before dereferencing, and construct a safe failure message for the throwable == null case so failure retries are still scheduled.
| if (throwable != null || !reply.isSuccess()) { | |
| handleFetchFailure( | |
| "Fetch failure (" + throwable.getClass().getSimpleName() + "). Retry in " + BAZAAR_DATA_SETTINGS.FAILURE_RETRY_MS + "ms", | |
| if (throwable != null || reply == null || !reply.isSuccess()) { | |
| String errorType; | |
| if (throwable != null) { | |
| errorType = throwable.getClass().getSimpleName(); | |
| } else if (reply == null) { | |
| errorType = "NullReply"; | |
| } else { | |
| errorType = "UnsuccessfulReply"; | |
| } | |
| handleFetchFailure( | |
| "Fetch failure (" + errorType + "). Retry in " + BAZAAR_DATA_SETTINGS.FAILURE_RETRY_MS + "ms", |
| CustomBazaarReply customReply = APIConversionUtil.fromSkyBlockReply(reply); | ||
|
|
||
| long snapshotTs = customReply.getLastUpdated(); | ||
| if (snapshotTs <= 0) { | ||
| PlayerActionUtil.notifyAll("Invalid lastUpdated <= 0. Retry in " + FAILURE_RETRY_MS + "ms", NotificationType.BAZAARDATA); | ||
| scheduleFetch(FAILURE_RETRY_MS); | ||
| handleFetchFailure("Invalid lastUpdated <= 0. Retry in " + BAZAAR_DATA_SETTINGS.FAILURE_RETRY_MS + "ms", false); | ||
| return; | ||
| } | ||
|
|
||
| if (snapshotTs != lastSnapshotTs) { | ||
| long previous = lastSnapshotTs; | ||
| lastSnapshotTs = snapshotTs; | ||
| currentReply = reply; | ||
| consecutiveIdenticalSnapshots.set(0); | ||
|
|
||
| EVENT_BUS.post(new BazaarDataUpdateEvent(reply)); | ||
|
|
||
| if (previous != -1) { | ||
| PlayerActionUtil.notifyAll("New snapshot " + snapshotTs + " (Δ " + (snapshotTs - previous) + " ms). Scheduling next predicted fetch.", NotificationType.BAZAARDATA); | ||
| } else { | ||
| PlayerActionUtil.notifyAll("First snapshot " + snapshotTs + " received.", NotificationType.BAZAARDATA); | ||
| } | ||
| consecutiveFailures.set(0); | ||
|
|
||
| scheduleNextFromSnapshot(snapshotTs); | ||
| } else { | ||
| int identical = consecutiveIdenticalSnapshots.incrementAndGet(); | ||
| PlayerActionUtil.notifyAll("Snapshot unchanged (" + snapshotTs + ") x" + identical, NotificationType.BAZAARDATA); | ||
| if (identical == STALE_WARNING_THRESHOLD) { | ||
| PlayerActionUtil.notifyAll("WARNING: " + identical + " identical snapshots in a row. Server might be lagging or BASE_INTERVAL_MS too short.", NotificationType.BAZAARDATA); | ||
| } | ||
| scheduleNextFromSnapshot(snapshotTs); | ||
| } | ||
| handleSnapshotResult(customReply, snapshotTs); | ||
| scheduleNextFromSnapshot(snapshotTs); |
There was a problem hiding this comment.
Exceptions thrown inside the whenComplete callback (e.g., from APIConversionUtil.fromSkyBlockReply(reply) or downstream conversion logic) are not caught by fetchOnceSafely(), so a single conversion failure can stop rescheduling future fetches. Wrap the callback body in a try/catch and route failures through the existing retry path (handleFetchFailure / scheduleFailureRetry).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Depends on #83
Split up BazaarDataManager class
Converts incoming api objects into wrapper objects
TODO:
Convert user's orders into
UserProductSummary's so they can be tracked in api dataCreate version of BazaarDataUtil.findItemPriceOptional that ignore's users orders
Make MarketPrices.getPriceForPosition use the above method to find the price ignoring user's orders (#78)